feat(storage): add resource span attributes for ACO ( App Centric Observability ) for async client - #16151
Conversation
product-auto-label
Bot
added
the
api: storage
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a BucketMetadataCache to cache bucket metadata (ID and location) and uses it to enrich OpenTelemetry spans with destination resource attributes in both TracingConnection and AsyncConnectionTracing. When metadata is not cached, a background fetch is triggered. The review feedback highlights several important issues: a critical data race on bg_tasks_ in TracingConnection::MaybeTriggerBackgroundFetch due to a lack of mutex protection during push_back, potential undefined behavior in BucketMetadataCache::Put if max_size is initialized to 0, unnormalized bucket names on permission denied errors in TracingConnection, and performance concerns regarding the expensive creation of a new StorageConnection on every background fetch in AsyncConnectionTracing.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16151 +/- ##
========================================
Coverage 92.30% 92.31%
========================================
Files 2223 2223
Lines 207876 208086 +210
========================================
+ Hits 191880 192085 +205
- Misses 15996 16001 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
9450ced to
db490d9
Compare
4244a97 to
0f627af
Compare
fa71a12 to
f18bad4
Compare
f18bad4 to
ab7a6df
Compare
| impl_->DeleteObject(std::move(p))); | ||
| } | ||
|
|
||
| std::shared_ptr<storage::AsyncRewriterConnection> RewriteObject( |
There was a problem hiding this comment.
RewriteObject is missing span enrichment. The actual RPCs and span creation happen downstream in rewriter_connection_tracing.cc, so it might be better to fix in a follow-up PR.
| auto reader = f.get(); | ||
| internal::DetachOTelContext(oc); | ||
| if (!reader) return internal::EndSpan(*span, std::move(reader).status()); | ||
| if (!reader) { |
There was a problem hiding this comment.
nit: Not sure why the formatting on this was changed. Typically, when there is an if without and else and it can all fit in < 80 characters, we leave it as one line.
I noticed similar changes were made elsewhere in the PR.
There was a problem hiding this comment.
I did not make those changes manually. It was done I think by checkers-pr. I fixed them.
…ervability ) for async client
ab7a6df to
cd81f2d
Compare
3 participants
No description provided.